-
Notifications
You must be signed in to change notification settings - Fork 11
Allow creating instances without waiting #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Allow creating instances without waiting #80
Conversation
Add `InstancesService.create_nowait` method that returns immediately after sending a create request to the API.
|
Good idea! Can this be rewritten to support the same syntax as for clusters, i.e. ...instance.create(
# Set to None to not wait for provisioning but return immediately
wait_for_status=verda_client.constants.cluster_status.PROVISIONING
) |
|
@huksley, even with
Here are a few possible ways to address this:
Please let me know if any of these options seem reasonable, or if you have alternative suggestions. |
|
Would it be ok if instance.create() with with wait_for_status=None would return instance populated with ID and values passed as request payload? that way, no get instance method will be needed |
|
@huksley, |
|
@huksley, I’ve added the I also had to add support for callables in |
verda/instances/_instances.py
Outdated
| payload['pricing'] = pricing | ||
| id = self._http_client.post(INSTANCES_ENDPOINT, json=payload).text | ||
|
|
||
| if not wait_for_status: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this completely change logic because it will not wait for status but immediately return instance without wait_for_status set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, wait_for_status is set to lambda s: s != InstanceStatus.ORDERED, which means the method doesn't return here and nothing changes for existing users that are not setting wait_for_status.
For those users that will decide to set wait_for_status=None, the method will return here immediately, which is the expected behavior.
The implementation is consistent with ClustersService.create.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR updates instance creation to optionally skip polling/waiting for a status transition after the create request.
Changes:
- Added a
wait_for_statuskw-only parameter toInstancesService.createto control whether/how to wait for a desired status. - Updated polling logic to support either a specific status or a predicate callable.
- Added unit tests covering different
wait_for_statusbehaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| verda/instances/_instances.py | Adds wait_for_status parameter and adjusts create polling/early-return logic. |
| tests/unit_tests/instances/test_instances.py | Adds parameterized tests validating new waiting behavior for create. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pricing: Pricing | None = None, | ||
| coupon: str | None = None, | ||
| *, | ||
| wait_for_status: str | Callable[[str], bool] | None = lambda s: s != InstanceStatus.ORDERED, |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR title/description says it adds InstancesService.create_nowait, but the diff only changes create() by introducing wait_for_status. Either add the described create_nowait method (e.g., a small wrapper calling create(..., wait_for_status=None)), or update the PR metadata to reflect the actual API change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the description
| pricing: Pricing | None = None, | ||
| coupon: str | None = None, | ||
| *, | ||
| wait_for_status: str | Callable[[str], bool] | None = lambda s: s != InstanceStatus.ORDERED, |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_for_status is typed as str | Callable[[str], bool], but the code/tests appear to use InstanceStatus values (and instance.status is compared to InstanceStatus). This should be typed consistently (e.g., InstanceStatus | Callable[[InstanceStatus], bool] | None) to avoid misleading API contracts and type-checking issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstanceStatus is a namespace for str constants. The actual type passed to wait_for_status is str and not InstanceStatus
| if callable(wait_for_status): | ||
| if wait_for_status(instance.status): | ||
| return instance | ||
| elif instance.status == wait_for_status: | ||
| return instance |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait_for_status is typed as str | Callable[[str], bool], but the code/tests appear to use InstanceStatus values (and instance.status is compared to InstanceStatus). This should be typed consistently (e.g., InstanceStatus | Callable[[InstanceStatus], bool] | None) to avoid misleading API contracts and type-checking issues.
| if callable(wait_for_status): | |
| if wait_for_status(instance.status): | |
| return instance | |
| elif instance.status == wait_for_status: | |
| return instance | |
| status_value = getattr(instance.status, 'value', instance.status) | |
| if callable(wait_for_status): | |
| if wait_for_status(status_value): | |
| return instance | |
| else: | |
| target_status = getattr(wait_for_status, 'value', wait_for_status) | |
| if status_value == target_status: | |
| return instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InstanceStatus is a namespace for str constants. The actual type passed to wait_for_status is str and not InstanceStatus
| contract: Optional contract type for the instance. | ||
| pricing: Optional pricing model for the instance. | ||
| coupon: Optional coupon code for discounts. | ||
| wait_for_status: Status to wait for the instance to reach, or callable that returns True when the desired status is reached. Default to any status other than ORDERED. If None, no wait is performed. |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grammar: change 'Default to' to 'Defaults to' for readability.
| wait_for_status: Status to wait for the instance to reach, or callable that returns True when the desired status is reached. Default to any status other than ORDERED. If None, no wait is performed. | |
| wait_for_status: Status to wait for the instance to reach, or callable that returns True when the desired status is reached. Defaults to any status other than ORDERED. If None, no wait is performed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current docstring is based on and consistent with ClustersService.create
Co-authored-by: Copilot <[email protected]>
Add
wait_for_statusparameter toInstancesService.create, similar toClustersService.create. Settingwait_for_status=Noneallows to return from the method immediately.Resolves #79